Changed Packages

Codecov Report

❌ Patch coverage is 92.10526% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.17%. Comparing base (c990755) to head (78b6164).
⚠️ Report is 18 commits behind head on main.
✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@
##             main    #3672      +/-   ##
==========================================
+ Coverage   54.12%   54.17%   +0.04%     
==========================================
  Files        2344     2346       +2     
  Lines       89539    89631      +92     
  Branches    25026    25063      +37     
==========================================
+ Hits        48460    48554      +94     
+ Misses      39517    39515       -2     
  Partials     1562     1562              

@HusneShabbir Lets firstly wait for @ShiranHi feedback, then only go for go for testing it. There are differences wrt to prototype like we are using PF popups and tried not overriding it much to look exactly as our prototype. Secondly in RHDH 2.1 we will not have the document summary, so we will be only showing documents name, that's why I added the lines between each doscuments.

/fs-review

🤖 Finished Review · ✅ Success · Started 9:07 AM UTC · Completed 9:21 AM UTC
Commit: 0186e63 · View workflow run →

Review

Findings

Medium

• [injection-vuln] workspaces/lightspeed/plugins/lightspeed/src/components/SourcesChipModal.tsx:133 — The handleSourceClick function passes the link value directly to window.open() without URL scheme validation. The link originates from backend data (doc_url via transformDocumentsToSources). If backend data were poisoned, a javascript: URI could execute arbitrary code in the user's browser context. The if (link) guard only checks truthiness, not the URL scheme.
  Remediation: Validate the URL protocol before calling window.open():

  const url = new URL(link, window.location.origin);
  if (!['http:', 'https:'].includes(url.protocol)) return;

• [logic-error] workspaces/lightspeed/plugins/lightspeed/src/components/SourcesChipModal.tsx:139 — When isExternal is falsy, handleSourceClick calls window.open(link, '_self'), which navigates the entire browser window away from the Backstage SPA. This causes the user to lose their chat session and application state. In practice, transformDocumentsToSources sets isExternal: !!doc?.doc_url, so any source with a URL will have isExternal=true — making this branch reachable only if isExternal is explicitly set to false while a link is provided. The likelihood is low, but the logic is semantically incorrect.
  Remediation: Either always open links in a new tab (_blank with noopener,noreferrer) or use client-side navigation for internal links.

Low

• [test-inadequate] workspaces/lightspeed/plugins/lightspeed/src/components/__tests__/SourcesChipModal.test.tsx — The test suite covers rendering, popover open/close, fallback titles, and source display, but does not verify window.open behavior when a source link is clicked. The handleSourceClick function contains branching logic (external vs non-external) that would benefit from test coverage.

• [insufficient-referrer-protection] workspaces/lightspeed/plugins/lightspeed/src/components/SourcesChipModal.tsx:139 — When isExternal is true, window.open is called with 'noreferrer' but not 'noopener'. Modern browsers treat noreferrer as implying noopener, but using 'noopener,noreferrer' would be defense-in-depth.

• [translation-cast-idiom] workspaces/lightspeed/plugins/lightspeed/src/components/SourcesChipModal.tsx:193 — The translation call uses (t as Function)(...). While this pattern exists in ~7 places in the notebooks area, the dominant codebase convention (~16 occurrences) is t('key' as any, { params }). Consider aligning with the more prevalent pattern.

• [naming-convention] workspaces/lightspeed/plugins/lightspeed/src/components/LightspeedChatBox.tsx:118 — The prop useSourcesChipModal reads like a React hook name (the use prefix is conventionally reserved for hooks). Other boolean props in this component use patterns like topicRestrictionEnabled, isStreaming. Consider renaming to showSourcesChipPopover or sourcesChipPopoverEnabled.

Thank you @its-mitesh-kumar, looks good to me. I understand that we are unable to remove the border line between each result or change the 'i' icon from purple to gray. If that’s a constraint, I am completely fine with this implementation. Thank you!

@ShiranHi

we are unable to remove the border line between each result or change

We can remove the border line between each result. I did introduced it knowingly to look better. In 2.1 we will be only having only document name. Let me know your preference.

change the 'i' icon from purple to gray.

We can change the 'i' icon from purple to gray. I made it purple since to me its look better particularly in light theme. Let me know your preference.

@ShiranHi Made both changes now

Light Theme

Dark Theme

Cc: @HusneShabbir

Implementation looks good.

Just one question: is there a reason we're using a different close button icon here? Wouldn't it be better to keep it consistent with the close icon used elsewhere?

.

@HusneShabbir this close icon is coming from PF popover. So lets not try to override it.

cc: @ShiranHi

@HusneShabbir this close icon is coming from PF popover. So lets not try to override it.

@its-mitesh-kumar do we have a bug ticket for this issue? It affects MCP config modal icons too, the x icon and key icon. We need to find a universal fix for this.

@HusneShabbir this close icon is coming from PF popover. So lets not try to override it.

@its-mitesh-kumar do we have a bug ticket for this issue? It affects MCP config modal icons too, the x icon and key icon. We need to find a universal fix for this.

@ciiay Since its PF close icon only. So lets use this only. At other places if we are importing close icon from MUI or somewhere else we can change that and use PF close icon throughout lightspeed.

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@ciiay Since its PF close icon only. So lets use this only. At other places if we are importing close icon from MUI or somewhere else we can change that and use PF close icon throughout lightspeed.

Not only the close icon. It happens to key icon on MCP config modal as well. Before the issue gets resolved in upstream, what can we do from our level? Do you think we need a jira ticket to track this issue?